fix(extractors): attribute same-file ES6 getter/setter property reads as call edges#2031
Open
carlos-alm wants to merge 1 commit into
Conversation
… as call edges A bare (non-call) property read or write on an ES6 get/set class accessor (`obj.isReady`, no call parens) invokes the accessor just as surely as `obj.isReady()` would — but call-site extraction only ever recognized member_expression nodes used as a call_expression's callee, so accessor reads/writes never produced a `calls` edge in either engine. Scoped to the same-file case: `this.prop` inside one of the accessor's own class's methods, or `varName.prop` where `varName`'s type (from the file's own typeMap) is a class also declared in the same file. A same-file accessor registry gates which bare property reads become synthetic Call entries, so they flow through the existing (unchanged) call-resolution cascade in both engines with no new edge kind, DB schema change, or serialization wiring. A property declaring both a getter and a setter is skipped (the two accessors share a qualified name and resolution can't tell them apart). Cross-file accessor recognition (the accessor's class declared in a different file than the read site) is filed as a follow-up: #2030. Fixes #1893 Impact: 12 functions changed, 11 affected
Contributor
Greptile SummaryThis PR fixes a coverage gap in call-edge extraction: bare property reads/writes on ES6
Confidence Score: 4/5Safe to merge. The change is additive (new synthetic call edges only), touches no existing resolution logic, and is well-tested across both engines. The break guard src/extractors/javascript.ts — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AST walk: member_expression node] --> B{parent is call_expression\nwith this as callee?}
B -->|yes| C[Skip — already handled\nby regular call path]
B -->|no| D{obj type?}
D -->|this| E[findParentClass → className]
D -->|identifier| F[typeMap lookup → className]
D -->|other| G[Skip]
E --> H{className.propName\nin localAccessors?}
F --> H
H -->|not found| I[Skip — not a same-file accessor]
H -->|found, both get+set| J[Skip — ambiguous target]
H -->|found, unambiguous| K{parent is plain\nassignment_expression?}
K -->|yes → setter| L{accessor has set?}
K -->|no → getter| M{accessor has get?}
L -->|no| N[Skip]
L -->|yes| O[Emit synthetic Call edge]
M -->|no| N
M -->|yes| O
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[AST walk: member_expression node] --> B{parent is call_expression\nwith this as callee?}
B -->|yes| C[Skip — already handled\nby regular call path]
B -->|no| D{obj type?}
D -->|this| E[findParentClass → className]
D -->|identifier| F[typeMap lookup → className]
D -->|other| G[Skip]
E --> H{className.propName\nin localAccessors?}
F --> H
H -->|not found| I[Skip — not a same-file accessor]
H -->|found, both get+set| J[Skip — ambiguous target]
H -->|found, unambiguous| K{parent is plain\nassignment_expression?}
K -->|yes → setter| L{accessor has set?}
K -->|no → getter| M{accessor has get?}
L -->|no| N[Skip]
L -->|yes| O[Emit synthetic Call edge]
M -->|no| N
M -->|yes| O
Reviews (1): Last reviewed commit: "fix(extractors): attribute same-file ES6..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const nameNode = methNode.childForFieldName('name'); | ||
| for (let i = 0; i < methNode.childCount; i++) { | ||
| const child = methNode.child(i); | ||
| if (!child || child === nameNode) break; |
Contributor
There was a problem hiding this comment.
child === nameNode uses object reference equality, which is always false in the tree-sitter WASM binding — as noted in the comment on collectAccessorPropertyRead in the very same PR: "tree-sitter (WASM) mints a fresh wrapper object on every childForFieldName()/parent access, so === … is always false." The Rust mirror correctly uses child.id() == name_node.id(). Currently harmless (the tree-sitter grammar places all get/set modifiers strictly before the name node, so no false-positive is possible), but the break is dead code on the WASM path and inconsistent with the .id-based comparisons elsewhere in this PR.
Suggested change
| if (!child || child === nameNode) break; | |
| if (!child || child.id === nameNode?.id) break; |
Contributor
Codegraph Impact Analysis12 functions changed → 11 callers affected across 1 files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A bare (non-call) property read or write on an ES6
get/setclass accessor (obj.isReady, no call parens) invokes the accessor just as surely asobj.isReady()would if written explicitly — but call-site extraction only ever recognizedmember_expressionnodes used as acall_expression's callee, so accessor reads/writes never produced acallsedge in either engine. Every getter/setter in a codebase (regardless of real usage) showedtotalDependents: 0/ roledead/dead-unresolvedviacodegraph fn-impact/codegraph roles --role dead.Root cause
Confirmed via
codegraph fn-impacton two same-file getters (NativeOrchestrationSession.isReady,SqliteRepository.db) — both showed zero callers despite real usage, ruling out a proximity/confidence issue: this was a pure extraction-coverage gap, not a resolution gap.Fix
Scoped to the same-file case:
this.propinside one of the accessor's own class's methodsvarName.propwherevarName's type (from the file's own typeMap) is a class also declared in the same fileA same-file accessor registry (
collectLocalAccessors/collect_local_accessors, built once per file from the file's ownget/setmethod_definitionnodes) gates which bare property reads become syntheticCallentries. Once gated, those entries are indistinguishable from a realthis.prop()/varName.prop()call and flow through the existing, unchanged call-resolution cascade in both engines — no new edge kind, no DB schema change, no new serialization across the WASM worker or native FFI boundary.obj.prop = value) invokes the setter; every other bare usage (reads, compound-assignment targets) invokes the getter.resolver/strategy.ts(resolveExactGlobalMatch) rather than risk an edge to the wrong accessor.src/extractors/javascript.ts(shared by both the WASM query-path and walk-path extraction, viarunCollectorWalk) andcrates/codegraph-core/src/extractors/javascript.rs(a new walk pass added aftertype_mapis populated, mirroring the existingmatch_js_call_assignmentsordering comment).Scope boundary
Cross-file accessor recognition (the accessor's class declared in a different file than the read site — the
SqliteRepository.dbrepro in #1893) needs a global accessor-kind registry and a DB schema change (to disambiguate get vs. set at resolution time across files), which is a larger, separate change. Filed as a follow-up: #2030.Test plan
npx vitest run tests/parsers/javascript.test.ts— 8 new unit tests (this-read, cross-var-read via typeMap, setter-write, ambiguous get+set skip, no duplicate on real calls, no false positive on plain method references, static accessor) — 246 passedcargo test --lib extractors::javascript::tests— 7 mirrored Rust unit tests — 175 passedtests/integration/issue-1893-getter-setter-property-read.test.ts: full-build WASM/native engine parity, incremental-vs-full-build parity, and the ambiguous get+set skip — all confirmed byte-identical across both engines (verified manually against the native addon rebuilt from this branch: 16 nodes / 21 edges on both engines and after an incremental rebuild)npm test— full suite green (3955 passed, 30 skipped, 2 todo, 0 failed)npx vitest run tests/benchmarks/resolution/resolution-benchmark.test.ts— 206 passed, no precision/recall regression across all 34+ language fixturescargo test --lib— 584 passednpm run lint— clean (only a pre-existing, unrelated fixture warning)fn-impact "Session.isReady"andfn-impact "Repo.db"now show 1 real dependent each (rolecore), matching between--engine wasmand--engine nativeStacked on #1892 (base branch
fix/issue-1892-constructor-calls-new-x-resolve-to-the-class) — only the javascript.ts/javascript.rs/test diff is this issue's change.